adding dynamic update of LED type dropdown #5014
Conversation
WalkthroughAdded a single function, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
DedeHai
changed the title
|
Looks like a good solution. 👍 |
fixed bug that allowed to change to 2-pin digital and then back to 1-pin digital.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
821-821: Consider improving readability of the analog pin selector.The selector construction is functionally correct but dense. The calculation
maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1and use of'A'.repeat()make the logic hard to follow at a glance.Consider adding a comment or extracting the calculation:
-disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`); +// Disable analog types that would exceed available pins +const availablePins = maxA - analogB + (isPWM(curType) ? numPins(curType) : 0); +const minPinsToDisable = availablePins + 1; +disable(`option[data-type^="${'A'.repeat(minPinsToDisable)}"]`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (3)
wled00/data/settings_leds.htm (3)
436-437: Good placement following reviewer feedback.The call to
updateTypeDropdowns()at the end ofUI()follows the reviewer's recommendation to call it once centrally rather than using event listeners.
798-823: Well-structured centralized approach.The two-pass strategy (counting current usage, then updating each dropdown) is sound and correctly handles the interdependencies between bus configurations.
808-808: Verify that virtual bus limiting is intentionally omitted.The
virtBcount is computed but never used to disable virtual bus types. This appears intentional based on comments elsewhere (line 454 mentions virtual buses are no longer limited), but please confirm that virtual buses truly have no restrictions.
wled00/data/settings_leds.htm
Outdated
| const enable = (q) => sel.querySelectorAll(q).forEach(o => o.disabled = false); | ||
| enable('option'); // reset all first | ||
| // max digital count | ||
| let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the maxDB calculation logic.
The arithmetic (!d.Sf["PR"].checked) * 8 - (!isS3()) produces unexpected results. When parallel I2S is enabled (PR.checked = true) and not on S3, this evaluates to 0 * 8 - 1 = -1, which increases maxDB to maxD + 1. This seems incorrect unless there's a specific hardware reason for allowing more digital buses in that configuration.
Additionally, using boolean-to-number coercion ((!condition) * 8) reduces readability compared to an explicit conditional.
Consider refactoring for clarity and correctness:
-let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
+let reduction = 0;
+if (is32() || isS2() || isS3()) {
+ if (!d.Sf["PR"].checked) reduction += 8; // parallel I2S disabled
+ if (!isS3()) reduction += 1; // additional reduction for non-S3
+}
+let maxDB = maxD - reduction;Please verify this matches the intended hardware constraints.
I need to investigate the actual code and its context to verify the review comment. Let me examine the repository.
🏁 Script executed:
#!/bin/bash
# Locate and examine the settings file around line 817
if [ -f "wled00/data/settings_leds.htm" ]; then
echo "=== Lines 810-825 of settings_leds.htm ==="
sed -n '810,825p' wled00/data/settings_leds.htm
echo ""
echo "=== Search for maxDB and maxD definitions/usage ==="
grep -n "maxDB\|maxD" wled00/data/settings_leds.htm | head -20
else
echo "File not found: wled00/data/settings_leds.htm"
find . -name "settings_leds.htm" -type f 2>/dev/null
fiLength of output: 1888
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about digital bus limits
echo "=== Search for LED bus, digital, or maxDB context ==="
grep -r "maxD\|maxDB\|digital.*bus\|parallel.*I2S" wled00/data/*.htm wled00/data/*.js 2>/dev/null | grep -i "comment\|//\|/\*" | head -10Length of output: 1353
🏁 Script executed:
#!/bin/bash
# Look for any firmware comments or constants related to bus/LED limits
echo "=== Checking for related documentation ==="
find wled00 -name "*.md" -o -name "*.txt" -o -name "*README*" | xargs grep -l "bus\|LED\|maxD" 2>/dev/null | head -5Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check git log for when this line was added and any commit messages
echo "=== Recent changes to settings_leds.htm around line 817 ==="
git log -p --follow -S "maxDB" -- "wled00/data/settings_leds.htm" 2>/dev/null | head -100Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search more broadly for the maxD/maxDB logic to understand intent
echo "=== Context search for PR.checked and isS3 patterns ==="
grep -n "PR.checked\|isS3\|is32\|isS2" wled00/data/settings_leds.htm | head -20Length of output: 796
🏁 Script executed:
#!/bin/bash
# Look for function definitions of is32, isS2, isS3
echo "=== Function definitions for board detection ==="
grep -n "function\s\+is32\|function\s\+isS2\|function\s\+isS3\|is32\s*=\|isS2\s*=\|isS3\s*=" wled00/data/settings_leds.htm | head -10Length of output: 317
🏁 Script executed:
#!/bin/bash
# Examine broader context around line 817 (200 lines before and after)
echo "=== Broader context: lines 750-850 ==="
sed -n '750,850p' wled00/data/settings_leds.htm | cat -n | tail -150 | head -100Length of output: 5155
🏁 Script executed:
#!/bin/bash
# Check if there are test files or comments explaining the maxDB formula
grep -r "maxD\|maxDB" wled00 --include="*.js" --include="*.htm" -B3 -A3 2>/dev/null | grep -E "(maxD|maxDB|//|comment)" | head -30Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Look at the file size and overall structure
echo "=== File structure check ==="
wc -l wled00/data/settings_leds.htm
echo ""
echo "=== Checking for any inline comments near maxDB calculation ==="
sed -n '800,830p' wled00/data/settings_leds.htmLength of output: 1566
🏁 Script executed:
#!/bin/bash
# Search for any explanation of what PR means in context
grep -n "Sf\[.PR.\]\|\"PR\"" wled00/data/settings_leds.htm -B2 -A2 | head -30Length of output: 1273
🏁 Script executed:
#!/bin/bash
# Look for pattern of boolean arithmetic to understand coding style
echo "=== Other boolean arithmetic in file ==="
grep -n "!\s*\w\+.*\*\s*[0-9]" wled00/data/settings_leds.htm | head -10Length of output: 143
🏁 Script executed:
#!/bin/bash
# Final check: extract the exact line 817 with full context
echo "=== Exact line 817 and surrounding lines ==="
sed -n '815,820p' wled00/data/settings_leds.htm | nl -v 815Length of output: 85
🏁 Script executed:
#!/bin/bash
# Check for any related issues or pull requests mentioning this calculation
echo "=== Search for hardware/LED bus documentation ==="
find wled00 -type f \( -name "*.cpp" -o -name "*.h" \) -exec grep -l "maxD\|digital.*bus" {} \; 2>/dev/null | head -5Length of output: 165
🏁 Script executed:
#!/bin/bash
# One more check - look for the actual values of maxD to understand the range
grep -n "maxD\s*=\|let maxD\|var maxD\|const maxD" wled00/data/settings_leds.htm -A2 | head -20Length of output: 1216
🏁 Script executed:
#!/bin/bash
# Verify the exact calculation in context - get 30 lines before line 817
sed -n '787,820p' wled00/data/settings_leds.htmLength of output: 1474
🏁 Script executed:
#!/bin/bash
# Search for any validation or constraints on maxDB after calculation
sed -n '815,835p' wled00/data/settings_leds.htmLength of output: 1039
🏁 Script executed:
#!/bin/bash
# Check if there's documentation in HTML comments
grep -n "<!--\|-->" wled00/data/settings_leds.htm | grep -A5 -B5 "bus\|maxD\|LED" | head -20Length of output: 0
I need to check the C++ firmware source to understand the hardware constraints behind this formula.
🏁 Script executed:
#!/bin/bash
# Look for parallel I2S constraints in C++ headers
echo "=== Checking const.h for parallel I2S and digital channel info ==="
grep -n "PARALLEL\|I2S\|LED_COUNT\|MAX.*DIG\|DMA" wled00/const.h | head -30Length of output: 1215
🏁 Script executed:
#!/bin/bash
# Search bus_manager.cpp for digital bus constraints
echo "=== bus_manager.cpp parallel I2S references ==="
grep -n "parallel\|PR\|maxD\|digital" wled00/bus_manager.cpp -i | head -40Length of output: 3022
🏁 Script executed:
#!/bin/bash
# Check for comments or logic around S2/S3 digital channel limits
echo "=== Searching for S2/S3 specific digital bus constraints ==="
grep -rn "S2\|S3\|ESP32-S" wled00/const.h wled00/bus_manager.cpp 2>/dev/null | grep -i "dig\|chan\|bus" | head -20Length of output: 2545
🏁 Script executed:
#!/bin/bash
# Look for any test or documentation about PR (parallel I2S) behavior
echo "=== Search for PR or parallel I2S documentation ==="
grep -rn "parallel.*I2S\|PR.*checked" wled00 --include="*.cpp" --include="*.h" -i | head -15Length of output: 1600
🏁 Script executed:
#!/bin/bash
# Check if there's any recent commit or comment explaining the maxDB logic
echo "=== Checking for explanation in code comments around digital channels ==="
grep -rn "maxDB\|max.*digital\|digital.*count" wled00 --include="*.cpp" --include="*.h" -B2 -A2 | head -40Length of output: 1501
🏁 Script executed:
#!/bin/bash
# Look at how maxDB value affects actual bus allocation
echo "=== Searching where maxDB variable/concept is used in logic ==="
grep -n "maxDB" wled00/data/settings_leds.htm -B3 -A3Length of output: 707
🏁 Script executed:
#!/bin/bash
# Check if there are any GitHub issues or PR comments mentioning this arithmetic
echo "=== Verify ESP32-S2 vs S3 digital I2S capability differences ==="
grep -rn "channel\|digital.*output" wled00 --include="*.h" | grep -E "S2|S3|16|12" | head -15Length of output: 1864
Fix the maxDB calculation to prevent allowing more digital buses than hardware supports.
The formula (!d.Sf["PR"].checked) * 8 - (!isS3()) produces incorrect results when parallel I2S is enabled. On ESP32 and S2 boards with PR checked, this evaluates to -1, making maxDB = maxD + 1. This allows the UI to show capacity for more digital buses than the hardware maximum.
The issue is inconsistent: S3 correctly returns maxD when PR is checked, but ESP32 and S2 incorrectly exceed their hardware limits.
Likely intended logic: reduce capacity by 8 when NOT using parallel I2S (when PR is unchecked). The subtraction of (!isS3()) appears to be attempting a per-board adjustment but uses subtraction instead of addition, inverting the logic.
Replace with:
-let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
+let maxDB = maxD - ((is32() || isS2() || isS3()) ? ((!d.Sf["PR"].checked) * 8) + (!isS3()) : 0);Or refactor for clarity:
-let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
+let reduction = 0;
+if (is32() || isS2() || isS3()) {
+ if (!d.Sf["PR"].checked) reduction += 8;
+ if (!isS3()) reduction += 1;
+}
+let maxDB = maxD - reduction;Verify that (!isS3()) reduction is intentional for S2/ESP32 when parallel I2S is enabled.
🤖 Prompt for AI Agents
wled00/data/settings_leds.htm around line 817: the current expression makes
maxDB larger when PR is checked on ESP32/S2 because (!d.Sf["PR"].checked) * 8 -
(!isS3()) can evaluate to -1; replace that sub-expression with a clear
conditional that subtracts 8 only when PR is unchecked (e.g. use
((!d.Sf["PR"].checked) ? 0 : 8)), and if any additional per-board adjustment for
non-S3 targets is required, apply it as an additive/subtractive term explicitly
(not by subtracting a boolean) and verify the intended (!isS3()) reduction for
ESP32/S2.
There was a problem hiding this comment.
@blazoncek this line is unchanged from previous code. is this the indended behaviour or is the rabbit right?
| Chip | PR.checked | Explanation | Effective subtraction | Result |
|---|---|---|---|---|
| ESP32/S2 | false (mono I²S) | Mono mode uses 8 busses | 8 - 1 = 7 | maxDB = maxD - 7 |
| ESP32/S2 | true (parallel) | Parallel mode → don’t reserve I²S busses | 0 - 1 = -1 | maxDB = maxD + 1 |
| ESP32-S3 | false (mono I²S) | Mono mode uses 8 busses | 8 - 0 = 8 | maxDB = maxD - 8 |
| ESP32-S3 | true (parallel) | Only parallel mode available | 0 - 0 = 0 | maxDB = maxD |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
S2 : 5/12
S3 : 4/12
32 : 9/16
These are supported configurations. First number is with parallel I2S disabled/unchecked.
There was a problem hiding this comment.
The formula above is correct.
|
Tested this and does not work as expected. |
|
I've come up with the solution: LTs.forEach((sel,i) => {
const last = i == LTs.length-1;
const curType = parseInt(sel.value);
const disable = (q) => sel.querySelectorAll(q).forEach(o => o.disabled = true);
const enable = (q) => sel.querySelectorAll(q).forEach(o => o.disabled = false);
enable('option'); // reset all first
// max digital count
let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
// disallow adding more of a type that has reached its limit
if (digitalB > maxDB && (!isDig(curType) || last)) disable('option[data-type="D"]');
if (twopinB > 2 && (!isD2P(curType) || last)) disable('option[data-type="2P"]');
disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`);
// we need to handle last added bus separately to properly select available type
if (last && sel.options[0].disabled) sel.selectedIndex = sel.querySelector('option:not(:disabled)').index;
}); |
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
Tested as is and does not work correctly.
Due to !isDig(curType) check. Which is mandatory with this approach.
Sorry, forgot to send submit.
wled00/data/settings_leds.htm
Outdated
| const enable = (q) => sel.querySelectorAll(q).forEach(o => o.disabled = false); | ||
| enable('option'); // reset all first | ||
| // max digital count | ||
| let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0); |
There was a problem hiding this comment.
The formula above is correct.
this is much cleaner than the version from my last commit and achieves the same. |
this actuall does not work properly, my version with setting to a "virtual bus" does work correctly. |
|
I initially went with virtual bus too but found it annoying. |
|
your version worked in my tests when removing: |
|
@netmindz once this is tested a bit more, it can be backported to 0.15 |
|
@DedeHai remove your last commit and move |
|
I've spent a better part of yesterday solving the issue without much succes while trying to keep existing behaviour. What I did manage to get working in the end is something like @DedeHai did with selecting non-demanding On/Off output when creating a new output. This output has no restrictions (apart from available GPIO) so can be selected at any time and allows AI generated code to function unchanged. However the place for The unfortunate consequence is that On/Off bus is selected as default when adding a new bus instead of usual 1st available type. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/data/settings_leds.htm (2)
516-520: Add null safety check for the option selection.The virtual bus approach is sound, but line 520 could throw an error if
querySelectorreturns null (e.g., if all options are somehow disabled).Apply this diff to add a defensive null check:
// temporarily set to virtual (network) type to avoid "same type" exception during dropdown update let sel = d.getElementsByName("LT"+s)[0]; sel.value = sel.querySelector('option[data-type="N"]').value; updateTypeDropdowns(); // update valid bus options including this new one -sel.selectedIndex = sel.querySelector('option:not(:disabled)').index; +let firstEnabled = sel.querySelector('option:not(:disabled)'); +if (firstEnabled) sel.selectedIndex = firstEnabled.index;
824-826: Consider adding explanatory comments for the type-limiting logic.The conditions correctly enforce bus type limits, but the analog calculation on line 826 is quite dense. A brief comment explaining the logic would help future maintainers.
For example:
// disallow adding more of a type that has reached its limit but allow changing the current type if (digitalB >= maxDB && !(isDig(curType) && !isD2P(curType))) disable('option[data-type="D"]'); if (twopinB >= 2 && !isD2P(curType)) disable('option[data-type="2P"]'); +// Disable PWM types that need more pins than available (accounting for current type's pins if PWM) disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (3)
wled00/data/settings_leds.htm (3)
270-270: Good placement of the updateTypeDropdowns() call.This correctly enforces type availability dynamically across all bus dropdowns whenever the UI refreshes.
822-822: Past review concern about maxDB calculation has been resolved.The formula has been confirmed correct by maintainers, who provided a table showing the intended behavior for ESP32/S2/S3 with/without parallel I2S. No changes needed.
Based on past review comments.
802-828: Nice implementation of dynamic type availability enforcement.The centralized approach cleanly replaces the previous "last bus only" restriction and correctly handles the various bus type limits across different ESP32 variants.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
824-828: Consider refactoring the analog pin selector for clarity.Lines 824-825 correctly enforce digital and 2-pin limits with appropriate current-type exceptions. Line 827 uses string repetition to generate a selector matching types that exceed available analog pins. While mathematically correct, the selector construction is cryptic.
Consider a more explicit approach:
- disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`); + // Disable PWM types requiring more pins than available + const availableAnalogPins = maxA - analogB + (isPWM(curType) ? numPins(curType) : 0); + for (let type of d.ledTypes) { + if (isPWM(type.i) && numPins(type.i) > availableAnalogPins) { + sel.querySelector(`option[value="${type.i}"]`)?.setAttribute('disabled', 'true'); + } + }Alternatively, add an inline comment explaining the repeat-string approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
🔇 Additional comments (2)
wled00/data/settings_leds.htm (2)
270-270: LGTM! Proper placement of dropdown update.Calling
updateTypeDropdowns()here ensures type availability is enforced whenever the UI is refreshed, correctly restricting options based on current bus configuration.
802-821: Well-structured bus type enforcement.The function correctly:
- Counts current bus usage across all types
- Resets all options before recomputing limits
- Calculates platform-specific digital bus limits accounting for parallel I2S configuration
The centralized approach eliminates the previous per-dropdown pre-disablement logic and ensures consistent enforcement across all selects.
- some string optimisations - imports from/replaces #5014
|
Sadly I can't just swap that base of this PR and if we are going to use this only for 0.15 then we need a fresh pr with these commits cheery picked @DedeHai |
|
I can create a new PR targeted for 0.15 |
|
@blazoncek is this "good enough" to be merged and backported to 0.15? You did mention that there should be a second place to update the list, aka sel.value = sel.querySelector('option[data-type="N"]').value;
updateTypeDropdowns(); // update valid bus options including this new one
sel.selectedIndex = sel.querySelector('option:not(:disabled)').index; |
TBH I never tested your approach though I borrowed some/most from it. Take a peek in my dev branch if you can take something from there. I did notice that selectedIndex = On/Off bus needs to be followed by updateDropdown, then selectedInde = first avaialble and updateDropdown again to work in every circumstance. See my code. IMO it should be an easy port to 0.15 as I only work on 0.15 codebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/data/settings_leds.htm (1)
516-521: Verify null safety for edge case.The temporary virtual-type approach correctly prevents false limit violations when adding a new bus. However, Line 520 assumes
querySelector('option:not(:disabled)')always returns an element.Apply defensive check as previously suggested:
- sel.selectedIndex = sel.querySelector('option:not(:disabled)').index; + const firstAvailable = sel.querySelector('option:not(:disabled)'); + if (firstAvailable) sel.selectedIndex = firstAvailable.index;
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
828-828: Consider adding a clarifying comment for the PWM pin calculation.The string-repeat selector pattern is clever but non-obvious. A brief comment would help future maintainers understand that the selector matches PWM types requiring more pins than available.
// Disable PWM types that need more pins than available (accounting for current type's pins if PWM) + // Selector matches types with data-type="A...A" where pin count > available (e.g., "AAAA" matches 4+ pin types) disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
🧠 Learnings (12)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-21T20:44:09.502Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4928
File: wled00/bus_wrapper.h:1248-1275
Timestamp: 2025-09-21T20:44:09.502Z
Learning: In WLED bus_wrapper.h, the _useParallelI2S flag is used for selecting the correct function pointer (between parallel I2S and non-parallel I2S bus types) rather than for changing memory calculations. Both parallel and non-parallel I2S configurations have the same memory requirements, so memUsage() correctly doesn't differentiate based on this flag.
Applied to files:
wled00/data/settings_leds.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
🔇 Additional comments (3)
wled00/data/settings_leds.htm (3)
270-270: Good placement for dropdown enforcement.Calling
updateTypeDropdowns()at the start of the LED fields section ensures type availability is enforced before UI elements are configured. This correctly implements the dynamic update behavior.
461-461: LGTM - enables dynamic dropdown updates on type change.The onchange handler correctly calls
updateTypeDropdowns()beforeUI(true), ensuring all dropdowns reflect updated bus type limits whenever the user changes any LED type.
803-830: Well-structured implementation of dynamic dropdown enforcement.The function correctly:
- Counts current bus usage by type (digital, analog, 2-pin, virtual)
- Enables/disables options across all dropdowns based on hardware limits
- Allows changing the current bus type even when limits are reached
- Accounts for PWM pin requirements using string-repeat selector pattern
The maxDB calculation (line 823) correctly adjusts for mono/parallel I2S modes as confirmed in previous reviews.
Note: Per coding guidelines, remember to run
npm run buildto regenerate embedded headers after these web UI changes.
|
I did test the 0.15 backport and since it is "identical" this PR also gets my green light for merging. |
3 participants
removing restriction to only allow changing of last bus by dynamically updating the dropdown menus
@blazoncek is this a valid fix or did I misunderstand the issue?
Summary by CodeRabbit